Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prepare Worker for External DF SDK support (and a few DF bug fixes) #746

Closed
wants to merge 47 commits into from

Conversation

davidmrdavid
Copy link
Contributor

@davidmrdavid davidmrdavid commented Jan 29, 2022

Problem Statement

The Durable Functions (DF) SDK for PowerShell is currently tightly-coupled with the worker code. This results in two major problems:

  1. SDK improvements and patches cannot be released quickly, they're slowed down by the worker-release infrastructure
  2. Users cannot choose which version of the SDK they want to use, which makes breaking changes harder to implement.

Solution

This PR refactors the DF-initialization logic in the worker to allow for users to take a dependency on a DF SDK downloaded from the PowerShell gallery (i.e an external SDK). This sets the foundation to extract the DF SDK from the worker, thereby removing this tight-coupling issue.

In the future, we plan to remove the built-in DF SDK implementation in the worker so that using the external SDK becomes the default option. However, this would be a breaking change and so we have no plans to remove it in the short term.

Caveat: While this PR contains the logic to enable using the external SDK, the specific logic to enable its usage will remain commented out until the external SDK is released on the PowerShell gallery. Still, merging these changes soon will allow us to ensure that the refactoring we've done is safe and doesn't break existing internal-SDK users.

Other goals

In addition, this PR also fixes several known DF SDK issues. These are listed below:

These fixes should have probably deserved their own PRs. However, due to time-pressure, they ended up being part of the same branch.

Description of Changes

Changes to enable the use of an external DF SDK

  • The Durable directory in the worker has been split into two folders: DurableSDK and DurableWorker. The former represents the SDK-specific components that, in the future, could be removed from this repo; this means it also represents the SDK-components that ought to exist in the external SDK. Meanwhile, the DurableWorker directory represents DF logic that is intrinsic to the worker, and ought to stay in this repo.

  • The new DurableSDK directory in particular is meant to be "drag and dropped" into an external SDK implementation; it's self-contained. As a result, we have removed any coupling of files within it with utility classes elsewhere in the repo. In practice, this means a few function definitions have been "duplicated" so that they now exist independently inside the "DurableSDK" directory. This is fine since we plan to eventually remove the DurableSDK repo from the worker.

  • The OrchestrationInvoker.Invoke method, where orchestration replay starts, now explicitely checks for whether an external DF SDK is enabled. If that were the case, it defers the driving of orchestration replay to the external SDK. It does this by invoking its "externalInvoker" reference.

  • In order to initialize the DF binding uniformly across the external and internal SDKs, we've delayed some of the binding initialization logic. In particular, the operations pwsh.AddParameter(orchestrationBindingInfo.ParameterName, ...) and pwsh.TracePipelineObject now occur inside the OrchestrationInvoker instead of in the Functions-general Invoke method. This allows the external SDK's Invoke method to control what data it passes to orchestrationBindingInfo.ParameterName.

Other changes

This PR also includes a few urgent bug fixes of the DF SDK. See them below.

  • We now expose a Get-DurableTaskResult utility which allows users to get the result of already-completed Tasks. This is needed because, if a user didn't immediately bind a task's result to a variable when "awaiting" it, they would previously lose access to it. The code for this was contributed by a community member here: Allow completed Tasks to expose their results #753.

  • Previously, WaitForExternalEvent tasks did not output its results to users. This has been fixed.

  • Previously, the fields InstanceId and IsReplaying where not exposed to users. Now they're available.

Future plans

After merging and releasing this PR, we'll look to continue the development of the external SDK, add an automated-CI to it, and eventually release it to the PowerShell Gallery.

co-authored to an equal extent with @michaelpeng36

using System.Management.Automation;
using Microsoft.Azure.Functions.PowerShellWorker.Durable.Tasks;

[Cmdlet("Get", "DurableTaskResult")]
Copy link
Contributor Author

@davidmrdavid davidmrdavid Mar 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the community-contributed CmdLet that allows users to get the result of an already-completed Task. In other PLs, users can easily do this by re-await/re-yielding that Task, but the PS programming model doesn't have that. The implementation of this CmdLet isn't as efficient as it could be, but I don't think that's something we can't address after merging it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This addition seems to be independent from the main purpose of the PR. If this is not very difficult, I recommend separating this into its own PR. Unless this is indeed too difficult.

Comment on lines -46 to -49
var loadedFunctions = FunctionLoader.GetLoadedFunctions();

var task = new ActivityInvocationTask(FunctionName, Input, RetryOptions);
ActivityInvocationTask.ValidateTask(task, loadedFunctions);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two were removed since they're incompatible with the external SDK. In addition, other DF SDKs don't perform this validation steps. If we want them, they ought to exist outside the DF SDKs.

I do agree that these are useful validations to have though, but if we perform these checks in the DF SDK components, then we're creating a strong dependency with worker-specific utilities.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intent of ActivityInvocationTask.ValidateTask is to provide user-friendly error messages when the specified activity function does not exits (for example, because of a typo in the name). Without this code, the error messages are really confusing and unhelpful. If you remove it, we need an adequate replacement.

Comment on lines 63 to 69
case HistoryEventType.EventRaised:
var eventRaisedResult = GetEventResult(completedHistoryEvent);
if (eventRaisedResult != null)
{
output(eventRaisedResult);
}
break;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This allows WaitForExternalEvent tasks to output a result

Comment on lines 212 to 226
public void GetTaskResult(
IReadOnlyCollection<DurableTask> tasksToQueryResultFor,
OrchestrationContext context,
Action<object> output)
{
foreach (var task in tasksToQueryResultFor) {
var scheduledHistoryEvent = task.GetScheduledHistoryEvent(context, true);
var processedHistoryEvent = task.GetCompletedHistoryEvent(context, scheduledHistoryEvent, true);
if (processedHistoryEvent != null)
{
output(GetEventResult(processedHistoryEvent));
}
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Part of the community-contributed logic for "Get-TaskResult". Again, this could use some caching, but I'd rather optimize that later since this is feature is urgently missing.

}
return null;
}

public static object ConvertFromJson(string json)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This used to exist in TypeExtensions, creating a tight-dependency with worker utilities. It's been copied here to allow this folder to be readily copied out into an external SDK

@davidmrdavid davidmrdavid changed the title [WIP] Separation of DF SDK code from Worker code. Prepare Worker for External SDK support Mar 17, 2022
@davidmrdavid davidmrdavid marked this pull request as ready for review March 17, 2022 17:50
@davidmrdavid davidmrdavid requested a review from AnatoliB March 17, 2022 17:50
@davidmrdavid davidmrdavid changed the title Prepare Worker for External SDK support Prepare Worker for External DF SDK support (and a few DF bug fixes) Mar 17, 2022
This was referenced Apr 7, 2022
@davidmrdavid
Copy link
Contributor Author

davidmrdavid commented Jul 12, 2022

Closing in favor of #839

@davidmrdavid davidmrdavid deleted the df-worker-and-sdk branch July 12, 2022 23:06
@davidmrdavid davidmrdavid mentioned this pull request Jul 18, 2022
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants